Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surface LegacyVersion and LegacySpecifier deprecation warnings #11945

Merged

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Apr 10, 2023

This is a simple approach to surface LegacyVersion and LegacySpecifier deprecation warnings, by patching our vendored copy of packaging. I open it to support discussion only. If it ends up to be valid, it would need to be converted into a vendoring patch.

towards #12063
refs #11715

There are two concerns with this approach

  1. It is not compatible with our vendoring policy (as mentioned by @pradyunsg in Upgrade the vendored packaging to 22.0+ #11715 (comment)).
  2. The warnings may lack context.

Regarding 1, it may be debatable, since we are "just" improving an existing deprecation warning and making it more visible?

Regarding 2, I'd need more test cases to see if there is enough context provided by surrounding messages or not. Please share them here if you have some. A simple example looks like this:

pip install --dry-run "pip>23.0.*"
DEPRECATION: This form of version specifier (>23.0.*) has been deprecated. pip 23.2 will enforce this behaviour change. A possible replacement is use PEP 440 compatible version specifiers.
Requirement already satisfied: pip>23.0.* in /home/sbi-local/.virtualenvs/tempenv-16fa522829ba/lib/python3.8/site-packages (23.1.dev0)

@sbidoul sbidoul changed the title [WIP] surface LegacyVersion deprecation warnings [WIP] surface LegacyVersion and LegacySpecifier deprecation warnings Apr 10, 2023
@sbidoul
Copy link
Member Author

sbidoul commented Apr 10, 2023

Related:

pip/src/pip/__main__.py

Lines 23 to 28 in 5d4a974

# Work around the error reported in #9540, pending a proper fix.
# Note: It is essential the warning filter is set *before* importing
# pip, as the deprecation happens at import time, not runtime.
warnings.filterwarnings(
"ignore", category=DeprecationWarning, module=".*packaging\\.version"
)

@sbidoul
Copy link
Member Author

sbidoul commented Apr 14, 2023

I tend to think we should do this in 23.1 rather than postponing.

Users of debundled distributions that don't have the packaging patch will see the warning only when raising PYTHONWARNINGS level. But if they don't, no harm is done.

Regarding the context, we can direct users to the deprecation issue that recommends raising verbosity, which hopefully should give enough context.

@sbidoul sbidoul added this to the 23.1 milestone Apr 14, 2023
@pfmoore
Copy link
Member

pfmoore commented Apr 14, 2023

The lack of discussion here, combined with the fact that 23.1 is due tomorrow, makes me feel like this isn't ready for 23.1. Sorry, I'd really like to get this resolved, but my understanding is that this will have enough of an impact that we can't simply drop it in and see what happens.

Personally, I'd like to just do this and accept the consequences. Yes, some users will be impacted, but we should take the position that they need to fix their projects. It's not up to pip to validate version numbers - projects using legacy versions should have been planning to transition for some time. But human nature being what it is, they won't have, and so pip is providing a 3-month "you need to fix this or things will break" warning to make sure they are aware, as per our deprecation policy. I think that's a reasonable thing to do and we should stick by our policy here.

But, based on past experience, it will cause complaints. And as such, I think we need to agree on this approach as a project. Specifically, if the warning causes problems for people, I expect with my RM hat on to say "sorry, this is a normal deprecation, deal with it". I explicitly don't want to revert the warning just because it's causing issues, and if we're not comfortable with that, then I think we need to address that disagreement before releasing the warning.

If we're not sure how much impact this will have, and that matters to us, then I think we need to work out how to collect that information properly, not by tentatively making the change and seeing if people complain too loudly. But that likely means looking at the question of telemetry, and opens up a whole other bunch of difficult issues, that I don't want to have the packaging upgrade blocked by. So I think we should continue with our current approach of making the best decision we can based on the limited information available, at least for this issue.

There's also a couple of technical issues to address:

  1. The PR is failing CI (and is still in draft state). It's definitely not going in unless that gets fixed.
  2. Do we know if this will pass the CPython test suite? We know they run with warnings as errors. I don't want to release 23.1 and then find we can't upgrade the CPython bundled version because of this.

@sbidoul
Copy link
Member Author

sbidoul commented Apr 14, 2023

Hi Paul, I'm not sure I get your point.

I think we all agree we need a deprecation cycle for this.

What I was trying to say is that I believe we'd better land an imperfect warning in 23.1 than waiting one more release to start the deprecation. The worse that can happen IMO is that we'll get questions of confused users because the warnings lack context and they don't know where the deprecated versions come from.

So the only thing we need to discuss here is whether the way this PR implements the warning is acceptable or not.

If it is, then we just need to convert this PR into a vendoring patch which should be easy to do.

The PR is failing CI

Because it is not a proper vendoring patch and it lacks a changelog entry. The rest is green.

(and is still in draft state).

As I said, I opened this to support the discussion.

Do we know if this will pass the CPython test suite? We know they run with warnings as errors. I don't want to release 23.1 and then find we can't upgrade the CPython bundled version because of this.

I don't know. We don't usually test pip within the cpython test suite before release, as far as I know. This PR, however, converts a python warning into a pip deprecation warning, so I guess it should be fine?

My 2 cents. Just trying to help. I don't personally have an urgency to upgrade our vendored packaging.

@pfmoore
Copy link
Member

pfmoore commented Apr 14, 2023

OK, sorry I might have misunderstood the status of this PR, based on the fact that you added it to the 23.1 milestone.

What are you expecting needs to happen to get this into 23.1? I assume you want someone to say that patching the vendored code like this is OK? It's fine by me, but I'm not clear what @pradyunsg's concerns are.

I agree I may be worrying too much about the CPython tests. Let's assume the best, we can fix things if there's a problem.

As long as we're all clear that once we start the deprecation, we're going through with it (I don't want another situation like dependency_links) then I guess it's just a question of managing user reactions (and again, I'm not that familiar with the risks here, I'm mostly just assuming that because we've hesitated so long on this, it's because there's an expectation somewhere that it might have a relatively large impact...)

Also, what (if anything) do you want from me as RM? Do you need a delay in 23.1?

@pradyunsg
Copy link
Member

I'd say we can defer this by 3 months; instead of trying to rush it into the 23.1 release. There's little to no reason for us to take on additional stress to hit a deadline here.

I'm not clear what @pradyunsg's concerns are.

We know that our code is debundled by downstream users and I'd prefer to not have deprecation patches be something we inject into our dependencies. What we tend to have are patches for making stuff run or vendorable, rather than new functionality or behaviour changes.

Behaviour changes mean we're effectively maintaining a fork of the dependency and that's not a particularly sustainable ways to doing things for us.

@sbidoul sbidoul removed this from the 23.1 milestone Apr 14, 2023
@sbidoul
Copy link
Member Author

sbidoul commented Apr 14, 2023

Ok, perfect. I've removed this from the 23.1 milestone.

As long as we're all clear that once we start the deprecation, we're going through with it

In my understanding, we have no choice. Since this blocks the packaging upgrade, we'll have to do this sooner or later.

@pfmoore
Copy link
Member

pfmoore commented Apr 14, 2023

Ok, perfect. I've removed this from the 23.1 milestone.

Thanks. Sorry for pushing back this hard, but I'd rather we do it right than too soon.

Since this blocks the packaging upgrade, we'll have to do this sooner or later.

Absolutely. I'd just like to avoid associating "enforcing standards" with user pain and frustration as much as we can.

@sbidoul
Copy link
Member Author

sbidoul commented May 29, 2023

Looking back at this, I still think this PR, when modified to be a proper vendoring patch, is a reasonable and simple way to surface warnings about LegacyVersion and LegacySpecifier. While I do understand the theory of not modifying vendored dependencies I don't quite see how it is a problem in this case, since all it does is tweaking warning.

@pfmoore
Copy link
Member

pfmoore commented May 29, 2023

Agreed, I'm fine with this going in (and I think that early in the 23.2 cycle is the right time). And I don't think we should worry about it not technically being in line with our patching policy - it's a temporary change to ensure we warn of a deprecation, and that seems perfectly reasonable to me.

@sbidoul sbidoul force-pushed the legacy-version-and-specifier-deprecation-sbi branch from 4e514db to 09a6a68 Compare May 31, 2023 07:04
@sbidoul sbidoul force-pushed the legacy-version-and-specifier-deprecation-sbi branch from da2ae13 to 09a6a68 Compare May 31, 2023 07:14
@sbidoul sbidoul added this to the 23.2 milestone May 31, 2023
@sbidoul
Copy link
Member Author

sbidoul commented May 31, 2023

Alright, so:

If anyone has the appetite to propose a more elaborate deprecation mechanism, we can still do so, but at least when this is merged we'll start receiving feedback.

@sbidoul sbidoul changed the title [WIP] surface LegacyVersion and LegacySpecifier deprecation warnings Surface LegacyVersion and LegacySpecifier deprecation warnings May 31, 2023
@sbidoul sbidoul marked this pull request as ready for review May 31, 2023 07:46
@sbidoul
Copy link
Member Author

sbidoul commented Jun 1, 2023

Does anyone know a public example with non-standard version numbers in dependencies? Before merging I'd like to test a few more cases than the trivial one shown in the OP, in particular to check if there would be a flood of identical warnings.

@uranusjr
Copy link
Member

uranusjr commented Jun 1, 2023

No immediate ones, but maybe look for some dependants to pytz? That’s a popular pacakge with non-standard versions until relatively recently. https://www.wheelodex.org/projects/pytz/rdepends/

@sbidoul
Copy link
Member Author

sbidoul commented Jun 1, 2023

Hm, so a simple pip install pytz logs a warning for each legacy version of pytz on PyPI then installs the last one. So these warnings are not going to be very useful for the user. I'm setting this back to draft.

@sbidoul sbidoul marked this pull request as draft June 1, 2023 08:44
@sbidoul
Copy link
Member Author

sbidoul commented Jun 1, 2023

Not sure how to handle this. Perhaps should we warn only when the resolver ends up selecting a legacy version?

@pfmoore
Copy link
Member

pfmoore commented Jun 1, 2023

To confirm, what will be the final result here? Once the deprecation becomes an error, will we simply ignore the legacy versions? If so, then maybe checking how many versions are invalid for each package and adding a warning saying something like "20 non-standard versions of pytz found, these will be ignored in future"?

It will make the reporting a lot more complex, and it's still not anything that needs action from the user unless an invalid version gets installed, but maybe it's useful information anyway, just to indicate how many invalid versions are out there?

The other thing we could do is have pip check (which I believe is run after any completed install?) validate whether all installed packages have valid version numbers. A message like "Your system now has XX non-standard versions installed, pip will no longer support these after version XX.Y". That would be something directly actionable by the user.

@sbidoul sbidoul force-pushed the legacy-version-and-specifier-deprecation-sbi branch from 5feb9c8 to 95f8f49 Compare June 25, 2023 11:23
@sbidoul
Copy link
Member Author

sbidoul commented Jun 25, 2023

Once the deprecation becomes an error, will we simply ignore the legacy versions

That's what I'm assuming.

What we could do is warn if any LegacyVersion is selected by the resolver (in pip install, pip wheel and pip download). I pushed a rough commit to show what it could look like in pip install.

To test that I'd very much welcome examples of packages with legacy versions on PyPI that are actually installable with a recent pip.

What about the legacy specifiers? When they are part of top level requirements, we will error out, I guess? But what will the resolver do legacy specifiers are found as part of Requires-Dist metadata?

@sbidoul
Copy link
Member Author

sbidoul commented Jun 25, 2023

I also pushed a commit with the deprecation in pip check.

@sbidoul sbidoul force-pushed the legacy-version-and-specifier-deprecation-sbi branch 2 times, most recently from b345823 to 1bc6d65 Compare June 26, 2023 20:40
@sbidoul sbidoul marked this pull request as ready for review June 26, 2023 20:44
@sbidoul sbidoul force-pushed the legacy-version-and-specifier-deprecation-sbi branch from 1bc6d65 to a45a74f Compare June 26, 2023 20:59
@sbidoul
Copy link
Member Author

sbidoul commented Jun 26, 2023

Interestingly recent versions of wheel and setuptools both vendor a recent packaging, so they can't build wheels with non standard versions and specifiers.

And PyPI has been validating PEP 440 for ages (dixit @dstufft), and also has a recent packaging.

So could it be we are worrying too much?

Anyway I updated this PR to warn

  • after resolving
  • in post-install conflict detection
  • in pip check

This is ready to review again. Let's land this in 23.2.

@dstufft
Copy link
Member

dstufft commented Jun 26, 2023

I looked it up, at least 8 years now, basically Warehouse has never allowed a version to be uploaded that wasn't PEP 440 compliant.

The old legacy codebase is too hard to code delve in to see if we validated PEP 440 there or not :)

@uranusjr
Copy link
Member

Seems like there’s a test failure. Not sure what’s going on.

[XPASS(strict)] Unexplained 'no module named platform' in https://github.com/pypa/wheel/blob/c87e6ed82b58b41b258a3e8c852af8bc1817bb00/src/wheel/vendored/packaging/tags.py#L396-L411

@sbidoul sbidoul force-pushed the legacy-version-and-specifier-deprecation-sbi branch from a45a74f to 782cff7 Compare June 27, 2023 14:43
@sbidoul
Copy link
Member Author

sbidoul commented Jun 27, 2023

Seems like there’s a test failure. Not sure what’s going on.

[XPASS(strict)] Unexplained 'no module named platform' in https://github.com/pypa/wheel/blob/c87e6ed82b58b41b258a3e8c852af8bc1817bb00/src/wheel/vendored/packaging/tags.py#L396-L411

I handled this in #12109. Not that I understand the cause any better than before, though :)

@pfmoore
Copy link
Member

pfmoore commented Jul 3, 2023

@sbidoul is this good to merge? You have 2 approvals, and the tests are passing.

@sbidoul
Copy link
Member Author

sbidoul commented Jul 3, 2023

It should be ready yes. The approvals were for a completely different approach, though. I'm away from the keyboard for two weeks so feel free to merge but note I will not be able to help with any issue until mid July.

@pfmoore
Copy link
Member

pfmoore commented Jul 3, 2023

Ah, I hadn't seen it was a different approach. I've just checked the new code and I'm OK with it. But I don't have the free time to commit to handling issues either, so I'm not going to merge it myself. @pradyunsg @uranusjr are either of you OK with merging this? Worst case, if any issues arise we can revert and push it back to 23.3, but I'd rather we unblock the packaging upgrade, so my preference is to have someone looking after it post-merge.

@pradyunsg pradyunsg merged commit b88adde into pypa:main Jul 3, 2023
@pradyunsg
Copy link
Member

I should have the bandwidth to pick up any issues. Thanks @sbidoul for driving this! ❤️

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2023
@sbidoul sbidoul deleted the legacy-version-and-specifier-deprecation-sbi branch April 7, 2024 15:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants